Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update single funded channel to select inputs before sending open #2894

Closed
wants to merge 3 commits into from

Conversation

remyers
Copy link
Contributor

@remyers remyers commented Aug 5, 2024

This PR selects the inputs for a single funded channel before sending the open message to our counter party. This change allows us to create more changeless transaction with the add_excess_to_recipient_position and max_excess coin selection options added by bitcoind PR #30080.

The makeFundingTx function now only selects inputs and change outputs, but does not sign the funding tx. The (new) signFundingTx function replaces the script of the funding output with a script that has the public keys for both parties and signs the funding tx.

This change passes `maxExcess` to Peer via OpenChannel and from Peer to Channel via INPUT_INIT_CHANNEL_INITIATOR. The `maxExcess` parameter will be used by bitcoind funding calls but is not currently used.
…ion calls

These parameters are only supported in a testing branch of bitcoind for now.
 - makeFundingTx now just selects inputs and change outputs
 - (new) signFundingTx replaces the funding output script and signs
@remyers
Copy link
Contributor Author

remyers commented Aug 5, 2024

@t-bast does this look like a reasonable approach to integrating the max_excess functionality into our funding flow?

@t-bast
Copy link
Member

t-bast commented Aug 6, 2024

As discussed offline, I don't think we'll want to merge this change for single-funded channel. For simplicity, we should only apply this strategy to dual-funded channels and splicing, which both currently use a similar funding flow (based on the InteractiveTxFunder) and will thus probably use the same design to handle excess. On top of that, we'll be deprecating the single-funded channels flow once enough nodes support dual funding, so it doesn't make much sense to have code churn on it now.

I'm not sure we'll be able to do something similar to what you've done for single-funded channels. I think the design should be that we spawn an InteractiveTxFunder outside of the InteractiveTxBuilder, before sending open_channel2 / accept_channel2 / splice_init / splice_ack whenever we want to contribute to the transaction, and then inject the resulting inputs/outputs when creating the InteractiveTxBuilder, but I haven't looked at the code changes that will be necessary to allow that.

@remyers
Copy link
Contributor Author

remyers commented Aug 6, 2024

I'm not sure we'll be able to do something similar to what you've done for single-funded channels. I think the design should be that we spawn an InteractiveTxFunder outside of the InteractiveTxBuilder, before sending open_channel2 / accept_channel2 / splice_init / splice_ack whenever we want to contribute to the transaction, and then inject the resulting inputs/outputs when creating the InteractiveTxBuilder, but I haven't looked at the code changes that will be necessary to allow that.

This is what I've been investigating - one aspect of dual funding that I can't seem to work around is that the initiator doesn't know whether or not to include unconfirmed inputs until after we receive 'accept_channel2' and check the 'requireConfirmedInputs' tlv.

Another approach is to create a workflow where we can abort and restart the dual-funded open or splice with the correct inputs locked if we find the local contribution should be increased to avoid a change output. Do you think something like that would be acceptable as a solution?

I need to investigate your RBF PR to better understand how that works, but I think we'll need a different approach to handle changing the local contribution for RBF.

@remyers
Copy link
Contributor Author

remyers commented Aug 6, 2024

Another idea is adding a tlv to the tx_add_input message that changes the local contribution previously set by open_channel2 (or accept_channel2). If the other side doesn't support that tlv it would cause the flow to break after tx_complete, which is ugly.

@t-bast
Copy link
Member

t-bast commented Aug 7, 2024

Another idea is adding a tlv to the tx_add_input message that changes the local contribution previously set by open_channel2 (or accept_channel2). If the other side doesn't support that tlv it would cause the flow to break after tx_complete, which is ugly.

I don't think this is a good idea: it clearly won't be accepted in the BOLTs, and is probably a can of worms to implement correctly.

This is what I've been investigating - one aspect of dual funding that I can't seem to work around is that the initiator doesn't know whether or not to include unconfirmed inputs until after we receive 'accept_channel2' and check the 'requireConfirmedInputs' tlv.

I don't think this is a big issue: we can use a heuristic to "guess" what our peer will set in accept_channel2 / splice_ack. Or we can restrict ourselves to only use confirmed inputs when we're the opener/splicer, which is safer.

Note that this is only an issue when we are sending open_channel2 / splice_init. But an important point with dual funding and liquidity ads is that we potentially want to add inputs when we receive open_channel2 / splice_init (which is the main scenario for LSPs where people purchase liquidity): in that case there's no problem, we have our peer's parameters. It's probably worth taking a look at #2848, which will likely be added to master before this excess feature. You can also take a look at the on-the-fly funding proposal to see the whole protocol flow: #2861.

Another approach is to create a workflow where we can abort and restart the dual-funded open or splice with the correct inputs locked if we find the local contribution should be increased to avoid a change output. Do you think something like that would be acceptable as a solution?

This would be very hacky and I don't think it works well in practice: you'd need to be able to correctly pipe that information between the interactive-tx session created by the channel to the peer, which is likely hard to get right. You'd also need to keep inputs locked while you're doing this, but handle all potential error cases to unlock them (otherwise we'd end up with permanently locked utxos which is a serious issue).

@t-bast
Copy link
Member

t-bast commented Sep 18, 2024

I don't think we should merge this for single-funded channel, let's focus on #2903 only instead.

@t-bast t-bast closed this Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants